Revert MAP implementation to `pycocotools` backend #1832

SkafteNicki · 2023-06-08T09:17:04Z

What does this PR do?

Fixes #1024
Fixes #1164

Reverts the MeanAveragePrecision implementation back to its v0.6 version for two primary reasons:

Our current implementation is orders of magnitude slower and even with improvements in Improve Mean Average Precision performance #1389 it is still slower
Nobody on the primary team is actually able to maintain the metric because we do not have experience with object detection. Therefore it is better to rely on pycocotools during the major calculations and we "just" provide an interface

The consequence for now is that pycocotools will be (re-)introduced as an require dependency for the metric.

TODO list:

Revert implementation
Make sure implementation supports argument iou_type (introduced after v0.6)
Make sure implementation supports argument iou_thresholds, rec_thresholds, max_detection_thresholds (introduced after v0.6)
Benchmark implementation to make sure it is still faster
Move from static testing to dynamic testing (for most tests) = call pycocotools directly during testing to check if output is correct
~~Check if implementation fixes Order of empty images in the batch changes the mAP results #1774~~
~~Check if implementation fixes Over-estimated map value #1793~~
~~Check if implementation fixes Wrong Calculation of Mean Average Precision #1184~~
Check if implementation fixes Discrepancy between values with metrics and detectron2 cocoeval #1164

Benchmark results below (measured on my laptop)

Time	This PR	Master	PR #1389 (improves current implementation)
Total time in init	0.0007	0.0010	0.0012
Total time in update	0.2445	0.2472	0.2769
Total time in compute	10.5286	324.6941	220.8867

As it been stated many times the old implementation is orders of magnitude faster than our current implementation.

Benchmark code

import time

import torch

try:
    from torchmetrics.detection import MeanAveragePrecision
except ImportError:
    from torchmetrics.detection import MAP
    MeanAveragePrecision = MAP

total_time = {}

class UpdateTime:
    def __init__(self, name) -> None:
        self._name = name

    def __enter__(self):
        self._start_time = time.perf_counter()

    def __exit__(self, exc_type, exc_val, exc_tb):
        end_time = time.perf_counter()
        if self._name in total_time:
            total_time[self._name] += end_time - self._start_time
        else:
            total_time[self._name] = end_time - self._start_time
        return True

def generate(n):
    boxes = torch.rand(n, 4) * 1000
    boxes[:, 2:] += boxes[:, :2]
    labels = torch.randint(0, 10, (n,))
    scores = torch.rand(n)
    return {"boxes": boxes, "labels": labels, "scores": scores}

with UpdateTime("init"):
    map = MeanAveragePrecision()

for _batch_idx in range(100):
    with UpdateTime("update"):
        detections = [generate(100) for _ in range(10)]
        targets = [generate(10) for _ in range(10)]
        map.update(detections, targets)

with UpdateTime("compute"):
    map.compute()

for name, time in total_time.items():
    print(f"Total time in {name}: {time}")

Before submitting

Was this discussed/agreed via a Github issue? (no need for typos and docs improvements)
Did you read the contributor guideline, Pull Request section?
Did you make sure to update the docs?
Did you write any new necessary tests?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

📚 Documentation preview 📚: https://torchmetrics--1832.org.readthedocs.build/en/1832/

Borda · 2023-06-08T11:53:31Z

can we rather preserve both so have MAPCoco?

SkafteNicki · 2023-06-08T12:26:44Z

can we rather preserve both so have MAPCoco?

Yes, we could do that, but then I would prefer we just add a backend="pycocotools"/"pytorch" argument to the class. That said, I am in the process of improving the testing and if the old implementation does not pass, then I would propose to still remove it.

Borda · 2023-06-08T12:45:23Z

I would prefer we just add a backend="pycocotools"/"pytorch" argument to the class

yes, that is also a good option. Just feel we have invested lots of effort to have our own implementation, so dropping it... :( also this would unblock users with exactly coco expectations (even if there are some known bugs)

SkafteNicki · 2023-06-08T13:59:34Z

I would prefer we just add a backend="pycocotools"/"pytorch" argument to the class

yes, that is also a good option. Just feel we have invested lots of effort to have our own implementation, so dropping it... :( also this would unblock users with exactly coco expectations (even if there are some known bugs)

We should not keep it just because we have invested time in it, that is just a sunk cost fallacy. We should keep it because we intent (with help) to make it stable.

…into new_map

ddelange · 2023-07-03T13:40:36Z

hi 👋 does this PR include breaking API changes?

can we still from torchmetrics.detection.mean_ap import MeanAveragePrecision and the API will be the same as before, just with the pycocotools backend?

Borda · 2023-07-03T13:44:37Z

can we still from torchmetrics.detection.mean_ap import MeanAveragePrecision and the API will be the same as before, just with the pycocotools backend?

yes, it shall be compatible 🦩
cc: @SkafteNicki

SkafteNicki · 2023-07-03T15:11:30Z

hi 👋 does this PR include breaking API changes?

can we still from torchmetrics.detection.mean_ap import MeanAveragePrecision and the API will be the same as before, just with the pycocotools backend?

@ddelange, no breaking changes :]

adamjstewart · 2023-07-06T15:35:22Z

no breaking changes :]

One breaking change :]

The following code:

class Foo(LightningModule):
    def __init__(self):
        self.metric = MeanAveragePrecision()

    def on_validation_epoch_end(self):
        self.log_dict(self.metric.compute())

used to work in 0.11.4. However, this PR adds a new classes key-pair to the metrics dict containing a list of all observed classes. log_dict now complains about this entry:

ValueError: `self.log(classes, tensor([0, 1], dtype=torch.int32))` was called, but the tensor must have a single element. You can try doing `self.log(classes, tensor([0, 1], dtype=torch.int32).mean())`

Here, tensor([0, 1]) is the classes value. Current workaround is to delete this key from the dict before logging, but it would be good to do this automatically.

ddelange · 2023-07-06T17:00:52Z

That's not this PR, it's a breaking change that was introduced in v1.0.0.rc0 ref ac64e63

adamjstewart · 2023-07-06T17:01:46Z

Good catch, thanks!

ddelange · 2023-07-06T17:05:33Z

No worries, I happened to know because I hit it in in a recent PR during a particularly representative case of dephell

SkafteNicki · 2023-07-07T06:32:25Z

@adamjstewart @ddelange
Yeah, I was probably too fast with saying no breaking changes :/
Would you prefer that we introduced an argument include_classes (or something similar) when initializing the class to enable/disable the inclusion of the class key.

For now you can also get around by a simple subclass:

from torchmetrics.detection.mean_ap import MeanAveragePrecision as MAP
class MeanAveragePrecision(MAP):
    def compute(self):
        res = super().compute()
        res.pop("classes")
        return res

adamjstewart · 2023-07-07T15:01:03Z

An argument would be good. I would vote for it to default to False so that the above example works out-of-the-box. I can submit a PR if you'd like.

SkafteNicki · 2023-07-08T14:16:04Z

An argument would be good. I would vote for it to default to False so that the above example works out-of-the-box. I can submit a PR if you'd like.

Please feel free to open a PR. I would prefer if we set the default to be True such that we do not break backwards compatible once again.

adamjstewart · 2023-07-08T17:50:45Z

If it defaults to True then the above code will continue to not work. The purpose of a PR would be to restore backwards compatibility, not to double down on the change.

refactor

9058fee

SkafteNicki added bug / fix Something isn't working enhancement New feature or request test / CI testing or CI labels Jun 8, 2023

SkafteNicki added this to the future milestone Jun 8, 2023

SkafteNicki requested review from Borda and justusschock as code owners June 8, 2023 09:17

tests

48f3788

ddelange mentioned this pull request Jun 9, 2023

Add support for python 3.11 autogluon/autogluon#3190

Merged

Borda and others added 10 commits June 12, 2023 13:49

Merge branch 'master' into new_map

8055b8e

working reference

b586a42

fix dtype casting

b8c99e7

remove old code

13fd068

improve docs

3c6f01c

Merge branch 'master' into new_map

46f1f8a

changelog

6132051

refactor

908f658

fix doc formatting

fd21da0

Merge branch 'master' into new_map

5efc562

SkafteNicki mentioned this pull request Jun 13, 2023

Wrong Calculation of Mean Average Precision #1184

Closed

SkafteNicki added 2 commits June 13, 2023 14:34

mypy

8c0a41d

Merge branch 'master' into new_map

5f993e4

SkafteNicki mentioned this pull request Jun 14, 2023

Order of empty images in the batch changes the mAP results #1774

Closed

Merge branch 'master' into new_map

8c6748d

SkafteNicki requested a review from stancld as a code owner June 14, 2023 08:51

skip doctest on missing import

5651da0

Merge branch 'master' into new_map

3d244dc

Borda requested a review from a team June 30, 2023 17:49

Borda approved these changes Jun 30, 2023

View reviewed changes

SkafteNicki added 2 commits July 1, 2023 15:21

ignore mypy

f5ba0fd

Merge branch 'new_map' of https://github.com/PyTorchLightning/metrics …

c1dfb27

…into new_map

mergify bot added the ready label Jul 1, 2023

Borda requested a review from maximsch2 July 3, 2023 07:40

Borda enabled auto-merge (squash) July 3, 2023 07:40

Merge branch 'master' into new_map

b75056d

justusschock approved these changes Jul 3, 2023

View reviewed changes

Merge branch 'master' into new_map

94587ad

Merge branch 'master' into new_map

bad9952

Merge branch 'master' into new_map

40823c4

Borda merged commit 5cc05c3 into master Jul 3, 2023

Borda deleted the new_map branch July 3, 2023 17:44

Borda modified the milestones: v1.0.0, v1.0.x Jul 10, 2023

zhiqiangdon mentioned this pull request Jul 26, 2023

[AutoMM] Upgrade torchmetrics to 1.0 autogluon/autogluon#3422

Merged

adamjstewart mentioned this pull request Sep 2, 2023

Refactor trainers microsoft/torchgeo#1541

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert MAP implementation to `pycocotools` backend #1832

Revert MAP implementation to `pycocotools` backend #1832

SkafteNicki commented Jun 8, 2023 •

edited

Loading

Borda commented Jun 8, 2023

SkafteNicki commented Jun 8, 2023

Borda commented Jun 8, 2023

SkafteNicki commented Jun 8, 2023

ddelange commented Jul 3, 2023

Borda commented Jul 3, 2023

SkafteNicki commented Jul 3, 2023

adamjstewart commented Jul 6, 2023

ddelange commented Jul 6, 2023 •

edited

Loading

adamjstewart commented Jul 6, 2023

ddelange commented Jul 6, 2023

SkafteNicki commented Jul 7, 2023 •

edited

Loading

adamjstewart commented Jul 7, 2023 •

edited

Loading

SkafteNicki commented Jul 8, 2023

adamjstewart commented Jul 8, 2023 •

edited

Loading

Revert MAP implementation to pycocotools backend #1832

Revert MAP implementation to pycocotools backend #1832

Conversation

SkafteNicki commented Jun 8, 2023 • edited Loading

What does this PR do?

Did you have fun?

Borda commented Jun 8, 2023

SkafteNicki commented Jun 8, 2023

Borda commented Jun 8, 2023

SkafteNicki commented Jun 8, 2023

ddelange commented Jul 3, 2023

Borda commented Jul 3, 2023

SkafteNicki commented Jul 3, 2023

adamjstewart commented Jul 6, 2023

ddelange commented Jul 6, 2023 • edited Loading

adamjstewart commented Jul 6, 2023

ddelange commented Jul 6, 2023

SkafteNicki commented Jul 7, 2023 • edited Loading

adamjstewart commented Jul 7, 2023 • edited Loading

SkafteNicki commented Jul 8, 2023

adamjstewart commented Jul 8, 2023 • edited Loading

Revert MAP implementation to `pycocotools` backend #1832

Revert MAP implementation to `pycocotools` backend #1832

SkafteNicki commented Jun 8, 2023 •

edited

Loading

ddelange commented Jul 6, 2023 •

edited

Loading

SkafteNicki commented Jul 7, 2023 •

edited

Loading

adamjstewart commented Jul 7, 2023 •

edited

Loading

adamjstewart commented Jul 8, 2023 •

edited

Loading